Skip to content

POC: start adding "repro snippets" for failed assertions #384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ev-br
Copy link
Member

@ev-br ev-br commented Jun 6, 2025

It is not always easy to tell what exactly failed with hypothesis (gh-379). Thus add a failing snippet to the exceptions' output.

With this PR, the OP example from gh-379 preserves whatever output pytest and hypothesis generated, and adds an explicit incantation that triggered the error:

$ ARRAY_API_TESTS_MODULE=array_api_compat.numpy pytest array_api_tests/test_manipulation_functions.py::test_repeat

... snip ...

array_api_tests/test_manipulation_functions.py:288: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
array_api_tests/test_manipulation_functions.py:318: in test_repeat
    out = xp.repeat(x, repeats, **kw)
../../miniforge3/envs/array-api/lib/python3.12/site-packages/numpy/_core/fromnumeric.py:506: in repeat
    return _wrapfunc(a, 'repeat', repeats, axis=axis)
../../miniforge3/envs/array-api/lib/python3.12/site-packages/numpy/_core/fromnumeric.py:66: in _wrapfunc
    return _wrapit(obj, method, *args, **kwds)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

obj = array([], dtype=bool), method = 'repeat', args = (array([], dtype=uint64),)
kwds = {'axis': None}
conv = <numpy._core._multiarray_umath._array_converter object at 0x7fd5f2252c10>
arr = array([], dtype=bool)

    def _wrapit(obj, method, *args, **kwds):
        conv = _array_converter(obj)
        # As this already tried the method, subok is maybe quite reasonable here
        # but this follows what was done before. TODO: revisit this.
        arr, = conv.as_arrays(subok=False)
>       result = getattr(arr, method)(*args, **kwds)
E       TypeError: Cannot cast array data from dtype('uint64') to dtype('int64') according to the rule 'safe'
E       
E       ========== FAILING CODE SNIPPET:
E       xp.repeat(array([], dtype=bool),array([], dtype=uint64), **kw) with kw={}
E       ====================
E       
E       Falsifying example: test_repeat(
E           x=array([], dtype=bool),
E           kw={},
E           data=data(...),
E       )
E       Draw 1 (repeats): array([], dtype=uint64)

../../miniforge3/envs/array-api/lib/python3.12/site-packages/numpy/_core/fromnumeric.py:46: TypeError
=============================== short test summary info ================================
FAILED array_api_tests/test_manipulation_functions.py::test_repeat - TypeError: Cannot cast array data from dtype('uint64') to dtype('int64') according ...
================================== 1 failed in 0.24s ===================================

The approach here is a bit manual, and I don't see a generic way to make it less so.

@ev-br
Copy link
Member Author

ev-br commented Jun 6, 2025

One more example:

$ ARRAY_API_TESTS_MODULE=array_api_compat.torch pytest array_api_tests/test_array_object.py::test_getitem

.... snip ....


array_api_tests/test_array_object.py:78: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

shape = (0, 2), dtype = torch.bool, data = data(...)

    @given(shape=hh.shapes(), dtype=hh.all_dtypes, data=st.data())
    def test_getitem(shape, dtype, data):
        zero_sided = any(side == 0 for side in shape)
        if zero_sided:
            x = xp.zeros(shape, dtype=dtype)
        else:
            obj = data.draw(scalar_objects(dtype, shape), label="obj")
            x = xp.asarray(obj, dtype=dtype)
        note(f"{x=}")
        key = data.draw(xps.indices(shape=shape, allow_newaxis=True), label="key")
    
        repro_snippet = ph.format_snippet(f"{x!r}[{key!r}]")
     ###   breakpoint()
    
        try:
>           out = x[key]
E           ValueError: step must be greater than zero
E           
E           ========== FAILING CODE SNIPPET:
E           tensor([], size=(0, 2), dtype=torch.bool)[(slice(None, None, None), slice(1, 0, -1))]
E           ====================
E           
E           Falsifying example: test_getitem(
E               shape=(0, 2),
E               dtype=torch.bool,  # or any other generated value
E               data=data(...),
E           )
E           x=tensor([], size=(0, 2), dtype=torch.bool)
E           Draw 1 (key): (slice(None, None, None), slice(1, 0, -1))
E           Explanation:
E               These lines were always and only run by failing examples:
E                   /home/br/repos/array-api-tests/array_api_tests/test_array_object.py:109

array_api_tests/test_array_object.py:92: ValueError


@ev-br
Copy link
Member Author

ev-br commented Jun 8, 2025

  • test_array_object.py
  • test_constants.py
  • test_creation_functions.py
  • test_data_type_functions.py
  • test_fft.py
  • test_has_names.py
  • test_indexing_functions.py
  • test_inspection_functions.py
  • test_linalg.py
  • test_manipulation_functions.py
  • test_operators_and_elementwise_functions.py
  • test_searching_functions.py
  • test_set_functions.py
  • test_signatures.py
  • test_sorting_functions.py
  • test_special_cases.py
  • test_statistical_functions.py
  • test_utility_functions.py

@ev-br
Copy link
Member Author

ev-br commented Jun 8, 2025

The shape of this POC is getting clear now. I've converted one of the test modules, test_manipulation_functions.py. The patch size is mostly whitespace/indentation, and is best viewed with the "hide whitespace" option.

Converting the rest is a bit of mechanical work, but is totally doable.

Downsides:

@cbourjau
Copy link
Contributor

I found myself wishing for this feature again today. The added DevEx far outweighs the obscured git blame, in my opinion. Is there anything blocking this?

It is not always easy to tell what exactly failed with hypothesis (data-apisgh-379).
Thus add a failing snippet to the exceptions' output.

For instance, run

$ ARRAY_API_TESTS_MODULE=array_api_compat.torch pytest array_api_tests/test_array_object.py::test_getitem

to see

E           ValueError: step must be greater than zero
E
E           ========== FAILING CODE SNIPPET:
E           tensor([False, False])[(slice(1, 0, -1),)]
E           ====================
@ev-br
Copy link
Member Author

ev-br commented Aug 12, 2025

Thanks for the ping @cbourjau !

  • Which function(s) did you hit this with?
  • Is the current use of repr appropriate for you? (it makes a difference in numpy, to tell numpy scalars apart from python scalars--- is the f"{arg!r}" good enough for ndonnx?)

I suppose the only thing blocking this is some elbow grease, would you be able to help? Would be great to split the set of test files, #384 (comment) --- the changes are pretty mechanical, just need to be test by test source file by source file.

The solution to the git blame issue is to add a .git-blame-ignore-revs file to the repo root:
https://docs.github.com/en/repositories/working-with-files/using-files/viewing-and-understanding-files#ignore-commits-in-the-blame-view, here's the SciPy usage: https://github.com/scipy/scipy/blob/main/.git-blame-ignore-revs

Therefore,

  • it's best to work source file by source file
  • to keep the number of entries under control, we should either use a commit per source file or a PR per source file + squash-merge.
  • we add the commit hashes to .git-blame-ignore-revs

@cbourjau
Copy link
Contributor

Coincidentally, I was looking at the test_repeat output today. __repr__ is fine for us. Doing one PR per file sounds better to me since it provides incremental value even if some files have not been migrated yet. I am happy to chime in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants